Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Moved kamon-netty #1113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

LeonDaniel
Copy link

No description provided.

Copy link
Contributor

@ivantopo ivantopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many tiny details to figure out here and there, and three bigger questions to address:

  • Is there a way this instrumentation can work with the latest Netty version? It will most certainly happen that some people will have newer versions in their classpath and as far as I understand, that would break this instrumentation. Some ideas that come to mind:
    • Find a different way of extracting the event loop name for the metrics
    • Add some conditions to ensure that event loop metrics will only be collected on supported versions and not fail on newer ones
  • There are no tests for epoll-based loops. I saw the helper methods in the tests side but they are not being used. Is it because this doesn't support epoll or the tests should be duplicated for epoll and that should be it?
  • What happens when someone is using a higher-level framework that builds on top of Netty (e.g. Armeria). Do we get double spans in that case? 🤔

* - task-waiting-time: The waiting time in the queue.
*/
val registeredChannelsMetric = Kamon.rangeSampler("netty.event-loop.registered-channels")
val taskProcessingTimeMetric = Kamon.histogram("netty.event-loop.task-processing-time", time.nanoseconds)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename this metric to netty.event-loop.task.duration. We will soon move to OTel-based conventions and for everything that measures a duration, the metric should end with duration.

import kamon.instrumentation.http.{HttpClientInstrumentation, HttpOperationNameGenerator, HttpServerInstrumentation}
import kamon.util.DynamicAccess

object Netty {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all the stuff that is commented out

@@ -0,0 +1,18 @@
package kamon.netty.instrumentation.advisor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all the code to the kamon.instrumentation.netty package to follow the same conventions across all projects


@OnMethodExit
static void onExit(@This Object eventLoop, @Return(readOnly = false) Queue<Runnable> queue) {
MonitoredQueue.apply((EventLoop) eventLoop, queue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the method that would fail on more recent Netty versions because newTaskQueue was turned into a static method?

@@ -0,0 +1,17 @@
<!DOCTYPE aspectj PUBLIC "-//AspectJ//DTD//EN" "http://www.eclipse.org/aspectj/dtd/aspectj.dtd">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file

}
}

class DefaultNameGenerator extends HttpOperationNameGenerator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this class for the same reasons as mentioned above

@@ -0,0 +1,8 @@
package kamon.netty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file

onSubTypesOf("io.netty.handler.codec.http.HttpObjectEncoder")
.advise(method("encode").and(takesArguments(3)), classOf[ServerEncodeMethodAdvisor])

// @After("execution(* io.netty.handler.codec.http.HttpObjectDecoder+.decode(..)) && args(ctx, *, out)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the commented code

onType("io.netty.bootstrap.ServerBootstrap$ServerBootstrapAcceptor")
.advise(method("channelRead").and(takesArguments(2)), classOf[ServerChannelReadMethodAdvisor])

// @Before("execution(* io.netty.bootstrap.ServerBootstrap.group(..)) && args(bossGroup, workerGroup)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove please

@@ -0,0 +1,26 @@
package kamon.netty.instrumentation.mixin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should be removed and use HasContext from instrumentation-common instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants